-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SPI RX/TX DMA and move to embedded-dma
(continuation of #165)
#230
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure i can provide much feedback on this, as i don't even have any hardware that uses SPI. I can do a small test of the UART part of it, when i get the time.
Other than that, it looks like a huge improvement in ergonomics, just from reading through it.
Thanks for having a look! And I must agree, the ergonomics significantly improved! :D |
embedded-dma
(continuation of #165)
FYI, I started with |
@David-OConnor Thanks for the input! And interesting choice of API. How come you chose to not use |
I'm on the fence on this and may eventually switch back, but it seems like these traits complicate user code without adding much value. Ie, I'm leaving it to the user to make sure the array's memory location is managed properly. For example, you could have a mutable buffer set up in the This API is still very much subject to change, but I'm keeping complexity to a minimum until specific abstractions (like |
Some overall thoughts overall after reading the code. 1: This is a solid addition, and improves the library by adding important functionality. Merging as-is would be great; could clean up in future PRs. 2: There are a number of abstractions here between the 3: Related to 2: You could document the reference manual steps using comments (perhaps verbatim) quotes, to improve maintainability. |
Thanks for the feedback! I'll add notes to the RM, these will indeed help. On the jumping between files, this is unfortunately the old structure of the HAL, would one want to change this one would need to do a major redesign of the internals. |
Signed-off-by: Leah <github.leah@hrmny.sh>
As it is a one-time setup, it does not make sense to re-setup the entire DMA engine, when it's enough to setup the buffer's address and length.
@David-OConnor I added references to the RM, is there something more we should add on that front? |
Looks great man! |
Super! I just need to test run the changes on HW so nothing weird sneaked in. |
I did some testing and mostly it looks good, but check the CS pin in the following image. This is generated with the following code, and I am not sure why it returns early so the pin can de-assert. let buf = unsafe {
static mut BUF: [u8; 5] = [0xf0, 0xaa, 0x00, 0xff, 0x0f];
&mut BUF
};
rprintln!("buf pre: 0x{:x?}", &buf);
cs.set_low().ok();
let transfer = dma_spi.transfer(buf);
let (buf, dma_spi) = transfer.wait();
cs.set_high().ok();
rprintln!("buf post: 0x{:x?}", &buf); The RX and TX DMA needs to say they have read/sent all data before |
I have checked a bit more, and the SPIs busy flag is also de-asserted before the transfer completes. |
I have now finished analyzing this and it's up to specification and by design as far as I understand. All internal flags happen with the sampling instant. After all this testing I'd like to say that this is ready for merge. |
Looks great! |
I think it look great! Awesome job on this! It's a huge step in the right direction! 👌 |
Thanks for all the reviews! |
Continuation of #165
Closes #123